-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Test only supported Groovy versions and automate their testing #269
Conversation
cb52ba6
to
de3ff91
Compare
Not sure you saw it, I was working on the same thing: #268 |
bd4cd16
to
83d70fa
Compare
defaults: | ||
run: | ||
shell: 'bash -o errexit -o nounset -o pipefail {0}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would you do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because what's often call the "strict" mode of Bash isn't on by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah but the bash is terminated right afterwards. It does not affect subsequent runs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For errexit
, It's true it won't affect other runs or jobs, but it would affect subsequent commands in the same job.
At the moment, we only have a single command, so it doesn't add any value, but if there were ever a second command added, the second command would execute even if the first command failed. That's usually not desirable.
nounset
was there to prevent us from accidentally using a variable that wasn't set.
pipefail
is another one that doesn't matter right now because we've only got 1 command taking from a pipe (and it's one I added with this PR).
I just had added this since it's what I put on all my Bash scripts (unless I have a specific reason not to).
.github/workflows/ci.yaml
Outdated
with: | ||
distribution: 'temurin' | ||
java-version: 11 | ||
java-package: jdk | ||
architecture: x64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I must have mis-read. I thought it was Zulu. I don't have a strong preference here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I meant arch: x64 is the default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I meant arch: x64 is the default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, gotcha. I had that from back in the day for whatever reason. (possibly no reason).
.github/workflows/ci.yaml
Outdated
run: | | ||
majorVersion=$(echo "${version}" | grep --extended-regex --only-matching "^[0-9]+") | ||
[ "${majorVersion}" -gt "3" ] && groupId="org.apache.groovy" || groupId="org.codehaus.groovy" | ||
./mvnw --batch-mode -DgroovyVersion="${version}" -DgroovyGroupId="${groupId}" clean install invoker:install invoker:run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'|' should not be used here, better use variables in github
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused how I could use variables in this case. The values are different, depending on which version of the job from the matrix is running. Ideally, I could include this along with the version in the matrix, but I didn't see a way to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it like that:
run: >-
./mvnw ${MAVEN_ARGS}
${{ matrix.groovy-version != '' && format('{0}{1}', '-DgroovyVersion=', matrix.groovy-version) || '' }}
${{ startsWith( matrix.groovy-version , '3' ) && '-DgroovyGroupId=org.codehaus.groovy' || '' }}
clean install invoker:install invoker:run
There are other ways, too. But on Github, the shell could change (e.g. ksh on AIX, zsh on Mac). So I would not want to rely on the shell.
So while you started working on your own PR AFTER I started the other one, you incorporated the changes from #266 which makes this PR way to big. Is this intended? |
I didn't see you'd already opened a PR. But I only opened this PR to experiment with what this might look like. That's why it's a draft. We'll probably merge other smaller PRs, and not actually this one. |
Okay. But on the bright side, we now have two different PRs so we can pick the things we like best from both. |
83d70fa
to
cb7dd29
Compare
cb7dd29
to
58721d2
Compare
No description provided.